Prometheus /metrics endpoint for proxy and proposal observability - #332
Prometheus /metrics endpoint for proxy and proposal observability#332wankhede04 wants to merge 2 commits into
Conversation
Adds an opt-in GET /metrics endpoint (AGENT_VAULT_METRICS_ENABLED) exposing
the standard Prometheus text exposition format, so operators running
Agent Vault as infrastructure can alert on/dashboard what the proxy is
actually doing instead of grepping debug logs or querying the request-log
table directly.
- internal/metrics: agent_vault_proxy_requests_total{service,status} and
agent_vault_proxy_request_duration_seconds, recorded via a
requestlog.Sink that stacks into the existing MultiSink alongside the
audit-log sink (no changes to the proxy hot path itself); plus
agent_vault_proposals{status} queried live from the store on every
scrape rather than tracked via incremented counters, so it can never
drift from whichever code path (approve/reject/expiry sweep) moved a
proposal between statuses
- store: new CountProposalsByStatus(ctx) global read, backing the gauge
- server: GET /metrics (404 until AttachMetrics is called — mirrors how
other optional server behavior is wired)
- docs: .env.example, environment-variables.mdx (new Prometheus metrics
section + metric reference table), reference/cli.mdx, README.md
Scoped to proxy + proposal metrics for this first pass; netguard-blocked
and rate-limit-rejected counters are natural follow-ups noted in the docs.
Closes Infisical#329
|
| Filename | Overview |
|---|---|
| internal/metrics/metrics.go | Adds the Prometheus registry, proxy metrics, scrape handler, and request-log sink adapter; concurrent gathers are not limited. |
| internal/metrics/proposals.go | Adds a live proposal collector whose database query uses an unbounded background context. |
| internal/store/sql_store.go | Adds a global grouped query for proposal counts with row and scan error handling. |
| cmd/server.go | Wires metrics into both server startup paths and combines the metrics and audit-log sinks. |
| internal/server/server.go | Adds optional metrics attachment and registers the public /metrics route. |
| internal/server/handle_metrics.go | Returns 404 when metrics are disabled and delegates enabled requests to the Prometheus handler. |
Reviews (1): Last reviewed commit: "feat: Prometheus /metrics endpoint for p..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Handler returns an http.Handler serving this Metrics instance's registry | ||
| // in the standard Prometheus text exposition format. | ||
| func (m *Metrics) Handler() http.Handler { | ||
| return promhttp.HandlerFor(m.registry, promhttp.HandlerOpts{}) |
There was a problem hiding this comment.
When metrics are enabled, unauthenticated callers can start unlimited concurrent gathers, each running the proposal count query with a background context. Repeated requests or disconnected clients can keep queries waiting for database connections and exhaust the pool, blocking normal application traffic.
GET /metrics is unauthenticated, so without a bound, a flood of scrapes could each open their own CountProposalsByStatus query and exhaust the database connection pool. Cache the result for a few seconds behind a mutex so concurrent/rapid scrapes share one in-flight query instead of each issuing their own, and cap the query itself with a timeout so a slow database can't pin every scrape indefinitely. On a transient query failure, serve the last good cached value instead of nothing.
|
Thanks for the review, @greptile-apps! Addressed the unbounded database-backed scrapes issue in fb0ce55:
Added |
|
The implementation is clean and correct. The key design choices all hold up: Mutex-held-for-duration (rather than a singleflight or separate "in-flight" flag) is the right call here — the 19 concurrent waiters serialise on one lock acquisition, each sees Stale-on-error semantics ( Timeout vs. TTL ordering ( One minor note: Everything else looks good — the concern is addressed. |
|
All Greptile review comments have been addressed and fixed. I take look on concerned Reported by greptile but that's not a major one, Would appreciate it if someone from the team could take a look and merge if it looks good. Thanks! |
1 participant
Closes #329.
What
Adds an opt-in
GET /metricsendpoint (AGENT_VAULT_METRICS_ENABLED) exposing the standard Prometheus text exposition format:agent_vault_proxy_requests_total{service,status}— proxied request outcomesagent_vault_proxy_request_duration_seconds— latency histogramagent_vault_proposals{status}— current proposal backlog by lifecycle status (pending,applied,rejected,expired), across all vaults — e.g.agent_vault_proposals{status="pending"}for the pending backlogDisabled by default; unauthenticated when enabled (consistent with
/healthand/v1/status) — meant to be scraped from a trusted network or behind a reverse proxy that adds auth, and clearly documented as such.Why
Agent Vault is meant to run as its own trust boundary / infrastructure (per the README), but there was no metrics export of any kind — only a debug log line per request or querying the request-log table directly. Neither wires into Prometheus/Grafana/Datadog, making it hard to alert on things operators actually care about (proposal backlog growing, latency regressions, error-rate spikes by service).
Design notes
requestlog.Sinkthat stacks into the existingrequestlog.MultiSinkalongside the audit-log sink — the package's docstring already describes this as the intended extension point ("Later sinks... stack here without touching the proxy"), so the hot path itself is untouched.CountProposalsByStatus) on every scrape rather than tracked via incremented counters at each transition call site. This is deliberate: proposal status changes happen from several places (approve, reject, the lazy expiry sweep), and a live query can never drift from theproposalstable regardless of which path fired — whereas scattering counter increments across those call sites risks silently undercounting if a path is missed (now or in a future change).Changes
internal/metrics:Metrics,EnabledFromEnv, therequestlog.Sinkadapter, and the live proposals collector.internal/store:CountProposalsByStatus(ctx)— a single globalGROUP BY statusquery.internal/server:GET /metrics, gated byAttachMetrics(404 until attached, same pattern as other optional server behavior).cmd: wiring in both the foreground and detached server-start paths..env.example,environment-variables.mdx(new Prometheus metrics section + metric reference table),reference/cli.mdx,README.md.Scoped to proxy + proposal metrics for this first pass, per the phased approach from the issue — netguard-blocked and rate-limit-rejected counters are natural follow-ups, noted in the docs as such. Happy to take those on separately.
Testing
internal/metrics(env parsing, sink recording, proposals gauge including the "store errors don't break the scrape" and "all four statuses always reported, even at zero" cases),internal/server(/metrics404-when-unattached vs 200-when-attached, live gauge values),internal/store(CountProposalsByStatusacross multiple vaults/statuses),cmd(env-gated wiring)./metrics404s by default, returns proper Prometheus output withAGENT_VAULT_METRICS_ENABLED=true.go build ./...,go vet ./...,go test ./...all pass (one pre-existing, unrelated failure ininternal/isolation— a Docker-socket-path test that also fails onmainin this sandbox).go.mod/go.sumdiff is minimal — onlygithub.com/prometheus/client_golangand its own transitive deps.